Skip to content

Refactor shortcut handler on iOS#22

Merged
luacmartins merged 16 commits into
Expensify:mainfrom
azimgd:ios-rewrite-uikeycommand
Apr 19, 2023
Merged

Refactor shortcut handler on iOS#22
luacmartins merged 16 commits into
Expensify:mainfrom
azimgd:ios-rewrite-uikeycommand

Conversation

@azimgd

@azimgd azimgd commented Apr 18, 2023

Copy link
Copy Markdown
Contributor

Rewriting ios implementation which apparently was using private API.

Following PR is relying on https://developer.apple.com/documentation/uikit/uikeycommand

@azimgd azimgd marked this pull request as ready for review April 19, 2023 06:49
@azimgd

azimgd commented Apr 19, 2023

Copy link
Copy Markdown
Contributor Author

@parasharrajat @luacmartins ready to be reviewed. npm version is changed to 1.0.0

@parasharrajat

Copy link
Copy Markdown
Member

Gonna run some tests.

@azimgd

azimgd commented Apr 19, 2023

Copy link
Copy Markdown
Contributor Author

@parasharrajat What's the ETA?

@parasharrajat

Copy link
Copy Markdown
Member

iOS:

So I tested it on Simulator and single key shortcuts are working fine. It will have to wait for some time to test it on a real device for all combinations.

Android:
All keys work. Sometimes Arrow keys don't log as it seems the parent scrollable container is being scrolled by pressing arrow keys. It is unrelated to this PR but worth mentioning.

Web:
Everything works. But [CMD+F] also triggers browser search.

How can we be sure that this code won't throw the same error as before? Is there a way to test that or encapsulating is enough?

@azimgd

azimgd commented Apr 19, 2023

Copy link
Copy Markdown
Contributor Author

@parasharrajat

parasharrajat commented Apr 19, 2023

Copy link
Copy Markdown
Member

Can you explain a little bit about what you are trying to say as well instead of just posting a link?

@azimgd

azimgd commented Apr 19, 2023

Copy link
Copy Markdown
Contributor Author

That's the documentation for the UIKeyCommand class in UIKit framework. UIKeyCommand is a public API and using it is not a violation of App Store guidelines.

@luacmartins luacmartins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM. I'll defer to @parasharrajat for testing.

@azimgd

azimgd commented Apr 19, 2023

Copy link
Copy Markdown
Contributor Author

@parasharrajat how much time would you need for tests? Personally tested both physical and simulator version.

@parasharrajat

Copy link
Copy Markdown
Member

@luacmartins Did you test the iOS shortcuts on a device? If so, then we don't have to wait here.

@parasharrajat parasharrajat left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM.

@parasharrajat

parasharrajat commented Apr 19, 2023

Copy link
Copy Markdown
Member

Question: can I test the dev build directly on iOS 16.4 device without an apple signing certificate from the apple developer program?

@azimgd

azimgd commented Apr 19, 2023

Copy link
Copy Markdown
Contributor Author

You mean building from your xCode and test on the real iPhone right? You have to have a signing certificate and a provisioning profile for that.

@parasharrajat

Copy link
Copy Markdown
Member

I selected Automatically manage signing from the Xcode settings. will that work?

To fasten the process here, @luacmartins can you please run the example app on the real device and test the multi-key shortcuts? I won't be able to do that for the next couple of hours.

@azimgd

azimgd commented Apr 19, 2023

Copy link
Copy Markdown
Contributor Author

That should work, yeap. Just letting you know that you can test multi-key shortcuts on simulator as well.
Make sure to enable “send keyboard input to device” under “I/O - Input” when simulator is running.

@parasharrajat

Copy link
Copy Markdown
Member

CMD+F does not work but others do. It might be linked to some native shortcuts of the simulator.

I think this is ready to merge.

@luacmartins luacmartins merged commit 8f008c1 into Expensify:main Apr 19, 2023
@luacmartins

luacmartins commented Apr 20, 2023

Copy link
Copy Markdown
Contributor

Published the new version to npm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants